-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore and support with solidus starter frontend #127
base: master
Are you sure you want to change the base?
Restore and support with solidus starter frontend #127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes and some notes in general:
- squash commits where possible, I see some smaller things like style changes that can be squashed
- Please be careful to separate what you work with (debug or developer tools) and what needs to be committed)
- circleci still gives an error, but I am sure that @kennyadsl or @jarednorman could take a look at that
@@ -1,4 +1,4 @@ | |||
/* | |||
*= require spree/frontend | |||
*= require spree/frontend/fontello |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this one, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kennyadsl I faced an issue after installing the extension i.e - somehow the sprockets is looking for the file spree/frontend
to be there when compiling assets, and raised an error with Sprockets::FileNotFound. I have to change this so that it points to an file instead of frontend directory as Sprockets is unable to find.
Let me know if there any better way to handle this.
def self.configured_providers | ||
::Spree::SocialConfig.providers.keys.map(&:to_s) | ||
end | ||
OAUTH_PROVIDERS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what's wrong with configured_providers
? Why do we need to change it into a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kennyadsl Earlier, we used to configure the providers using an initializer file, where we configure provider with credentials, I find it would be more appropriate if we pull the provider and credentials details from the Spree::AuthenticationMethod
table instead, so I removed this method.
Constant I took, just to maintain the list of provider into the extension itself.
Let me know your thoughts on this.
Thanks for the contribution! Spec failures seem related to the changes, I don't think they are passing locally either. |
Fix stylesheet to use correct path to support starter FE UI updates for the social logins
- Implement ability to use omniauth from authentication method table not from config - Remove solidus_social initializer from generator and added devise as initializer in extension - Added validations for api_key and api_secet - Fix for loading error for authentication method class Fix for loading error of user registration controller
Align icons with starter frontend solidusio-contrib#125 Fix button styling Minor fixes for social icons Fix for loading and missing routes - #155 - Fix ::Spree::UserRegistrationsController loading error - Fix for some missing routes
Social auth buttons UI fixes
7c2487b
to
cdab9f2
Compare
Thank you @kennyadsl. I ran specs locally in SQLite and it worked, so circleCI. But, while running the specs with postgres/mysql it not creating the database automatically and we need this as we load the provider config from table now. Any thoughts on how to fix this ? |
Issue: #126